Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add spec for platform exclusion #143

Merged
merged 9 commits into from
Jul 29, 2020

Conversation

terrajobst
Copy link
Contributor

@terrajobst terrajobst commented Jul 18, 2020

No description provided.

@jeffhandley jeffhandley requested a review from buyaa-n July 18, 2020 05:51
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good start to me. Some early thoughts.


```XML
<ItemGroup>
<SupportedPlatform Include="android" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at dotnet/sdk#12547, we already have an MSBuild item named SupportedTargetPlatform. Are we intending this to be the same thing as that? If not, these names are too similar to both be used.

cc @dsplaisted @sfoslund

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now we're using SupportedTargetPlatform as an itemgroup containing the supported TargetPlatformVersions that correspond to the specified TargetPlatformIdentifier.

Copy link
Contributor Author

@terrajobst terrajobst Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So no then 😄 . Your notion of supported is "what can this SDK produce" while mine is "what platforms is the author of this project file intending to support". We probably need a different name then.

Copy link
Member

@eerhardt eerhardt Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your notion of supported is "what can this SDK produce"

Should we rename the SDK itemgroup then? I think the name SupportedTargetPlatform sounds like it is what target platforms the current project supports, not necessarily what the SDK supports.

Copy link
Contributor Author

@terrajobst terrajobst Jul 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dsplaisted were should I file a bug to discuss renaming the SDK's SupportedTargetPlatform item group?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In dotnet/sdk.

Note that there was already a SupportedTargetFramework item which we use to populate the TargetFramework dropdown on the VS property page, so we went with the same pattern for SupportedTargetPlatform.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. In that sense I think we should come up with a new name for the analyzer. Let me chew on that.

@jeffhandley
Copy link
Member

I keep thinking about how the crux of the challenge we're having is that marking a specific OSPlatform as supported implies that no other platforms are supported, but we don't always want that behavior.

[MinimumOSPlatform("windows7.0")]
public void WindowsOnlyAPI() {}

[MinimumOSPlatform("ios14")]
public void WorksEverywhereButOnIosVersion14IsRequired() {}

If we set that problem aside for a moment, and we focus purely on excluding an OSPlatform, then I think we're otherwise OK. The following would just work, wouldn't it?

[UnsupportedOSPlatform("browser")]
public void WorksEverywhereExceptBrowser() {}

I believe the ranges were being introduced so that we could use [UnsupportedOSPlatform] to denote a combination of supported and unsupported, specifying which version ranges are unsupported, leaving anything outside the range as supported without implying exclusive platform support. Using [UnsupportedOSPlatform] in that way feels awkward to me, and less expressive than using [MinimumOSPlatform] to indicate support.

I'm really concerned about the range support, regardless of the syntax. It makes the data harder to reason over as a human code reader, and potentially more brittle when reasoning over in the analyzer implementation. I think we'd introduce a higher likelihood that the data will be incomprehensible and then the analyzer will need to introduce more diagnostics for when that's the case.

So what if we did the following?

  1. Rename RemovedInOSPlatformAttribute to UnsupportedOSPlatformAttribute, but otherwise leave it as-is
    • This specifies the lower-bound version in which an API became unsupported
  2. Add a property to MinimumOSPlatformAttribute that indicates whether or not the platform should be treated as exclusive

I think we could then satisfy our scenarios:

[MinimumOSPlatform("windows7.0")]
public void WindowsOnlyAPI() {}

[MinimumOSPlatform("ios14.0", IsExclusive = false)]
public void WorksEverywhereButOnIosVersion14IsRequired() {}

[UnsupportedOSPlatform("browser")]
public void WorksEverywhereExceptBrowser() {}

The evolution of an API going from unsupported to having a minimum version required for support would involve changing [UnsupportedOSPlatform("ios")] to [MinimumOSPlatform("ios14", IsExclusive = false)]. If there are multiple platforms that add support over time, then they would each need to specify IsExclusive = false.

To show an edge case where something comes in and out of support for a platform repeatedly:

[MinimumOSPlatform("ios12.0", IsExclusive = false)]
[UnsupportedOSPlatform("ios14.0")]
[MinimumOSPlatform("ios12.0", IsExclusive = false)]
[MinimumOSPlatform("ipados16.0", IsExclusive = false)]
public void Worked_In_12_And_13_But_Not_14_Or_15_But_Works_Again_In_16() {}

If this approach has merit, then I'd suggest that we also rename MinimumOSPlatformAttribute to SupportedOSPlatformAttribute. Both SupportedOSPlatform and UnsupportedOSPlatform would accept a platform name with an optional version such that the version is always treated as the lower-bound.

By default, a [SupportedOSPlatform] attribute denotes exclusive support on that platform. But specifying IsExclusive = false on the attribute allows support to be turned on for the platform without affecting support on other platforms. An [UnsupportedOSPlatform] attribute never implies anything about any other platforms.

To address the [concern of introducing another means for obsoleting an API]([MinimumOSPlatform("ios12.0", IsExclusive = false)]), I suggest we remove the ObsoletedInOSPlatformAttribute that we introduced.

To summarize, I propose that we:

  1. Rename RemovedInOSPlatformAttribute to UnsupportedOSPlatformAttribute
  2. Rename MinimumOSPlatformAttribute to SupportedOSPlatformAttribute
  3. Remove ObsoletedInOSPlatformAttribute
  4. Add an IsExclusive Boolean property to SupportedOSPlatformAttribute, that defaults to true

Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @terrajobst! I left some comments and primary have 2 topics that will need resolution:

  1. Should we use a params string[] platformNames constructor with the iOS family of platforms in mind?
  2. I think we had problems with the || based guards; I'll need @buyaa-n to weigh in there.

@buyaa-n
Copy link
Contributor

buyaa-n commented Jul 28, 2020

  1. I think we had problems with the || based guards; I'll need @buyaa-n to weigh in there.

This is a more meaningful scenario than the other || example we discussed before, for this one we should try to add support

@danmoseley danmoseley requested a review from Lxiamail July 28, 2020 19:01
terrajobst and others added 4 commits July 28, 2020 14:39
Co-authored-by: Jeff Handley <jeffhandley@users.noreply.github.com>
Co-authored-by: Jeff Handley <jeffhandley@users.noreply.github.com>
@terrajobst terrajobst merged commit c74fb9d into dotnet:master Jul 29, 2020
@terrajobst terrajobst deleted the platform-exclusion branch July 29, 2020 20:10
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants